Change BFT Sync to process block responses as expected#3615
Closed
kaimast wants to merge 1 commit intoProvableHQ:stagingfrom
Closed
Change BFT Sync to process block responses as expected#3615kaimast wants to merge 1 commit intoProvableHQ:stagingfrom
kaimast wants to merge 1 commit intoProvableHQ:stagingfrom
Conversation
kaimast
force-pushed
the
validator-advance-block-sync
branch
from
34d520d to
14bd53d
Compare
kaimast
force-pushed
the
validator-advance-block-sync
branch
from
14bd53d to
592672c
Compare
Collaborator
|
Changes make sense, as we do not want the validator nodes to call the sync functions of the internal Couple questions:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Stress testing revealed an error in the recent changes to block synchronization. When validators fall behind they do not sync block properly. Meaning, the sync blocks like clients, and do not add the contained certificates to the BFT module.
This PR provides a fix for that by modifying
Sync::advance_with_sync_blocksand adds more documentation to theSyncandBlockSyncstructs.It also renames two functions to make their functionality clearer:
Sync::sync_storage_with_blockswas changed toSync::try_advancing_block_synchronizationto make it obvious that this is the validator version ofBlockSync::try_advancing_block_synchronization.Sync::try_block_syncwas changed toSync::issue_block_requestsbecause it only create and sends them, but does not process any block responses.Finally, the PR adds function
Sync::try_block_syncwas added that encapsulates a full iteration of a validator's block synchronization. This function is invoked manually in some unit tests (mostly inPrimary's tests).Test Plan
We will run more stress tests before merging this into staging (thanks to @kpandl for doing most of the work here).
Related PRs
#3543 introduced the bug that this PR fixes.